[clang][deps] Properly capture the global module and '\n' for all module directives #148685

naveen-seth · 2025-07-14T17:37:32Z

Previously, the newline after a module directive was not properly captured and printed by clang::printDependencyDirectivesAsSource.

According to P1857R3, each directive must, after skipping horizontal whitespace, appear at the start of a logical line. Because the newline after module directives was missing, this invalidated the following line.
This fixes tests that were previously in violation of P1857R3, including Objective-C directives, which should also comply with P1857R3.

This also ensures that the global module fragment module; is captured by the dependency directives scanner.

With this change, the dependency directive scanner now properly identifies "module;" as a directive, as per P1857R3. Previously, the global module fragment was not recognized by the scanner.

llvmbot · 2025-07-14T17:38:05Z

@llvm/pr-subscribers-clang

Author: Naveen Seth Hanig (naveen-seth)

Changes

With this change, the dependency directive scanner now properly identifies "module;" as a directive, as per P1857R3.
Previously, the global module fragment was not recognized by the scanner.

Full diff: https://github.com/llvm/llvm-project/pull/148685.diff

2 Files Affected:

(modified) clang/lib/Lex/DependencyDirectivesScanner.cpp (+7)
(modified) clang/unittests/Lex/DependencyDirectivesScannerTest.cpp (+5-4)

diff --git a/clang/lib/Lex/DependencyDirectivesScanner.cpp b/clang/lib/Lex/DependencyDirectivesScanner.cpp
index d894c265a07a2..8822e760274d0 100644
--- a/clang/lib/Lex/DependencyDirectivesScanner.cpp
+++ b/clang/lib/Lex/DependencyDirectivesScanner.cpp
@@ -728,6 +728,13 @@ bool Scanner::lexModule(const char *&First, const char *const End) {
       return false;
     break;
   }
+  case ';': {
+    // Handle the global module fragment `module;`.
+    if (Id == "module" && !Export)
+      break;
+    skipLine(First, End);
+    return false;
+  }
   case '<':
   case '"':
     break;
diff --git a/clang/unittests/Lex/DependencyDirectivesScannerTest.cpp b/clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
index d2ef27155df94..92f6f401ec6b7 100644
--- a/clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
+++ b/clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
@@ -1122,16 +1122,17 @@ ort \
     )";
   ASSERT_FALSE(
       minimizeSourceToDependencyDirectives(Source, Out, Tokens, Directives));
-  EXPECT_STREQ("#include \"textual-header.h\"\nexport module m;"
+  EXPECT_STREQ("module;#include \"textual-header.h\"\nexport module m;"
                "exp\\\nort import:l[[rename]];"
                "import<<=3;import a b d e d e f e;"
                "import foo[[no_unique_address]];import foo();"
                "import f(:sefse);import f(->a=3);"
                "<TokBeforeEOF>\n",
                Out.data());
-  ASSERT_EQ(Directives.size(), 11u);
-  EXPECT_EQ(Directives[0].Kind, pp_include);
-  EXPECT_EQ(Directives[1].Kind, cxx_export_module_decl);
+  ASSERT_EQ(Directives.size(), 12u);
+  EXPECT_EQ(Directives[0].Kind, cxx_module_decl);
+  EXPECT_EQ(Directives[1].Kind, pp_include);
+  EXPECT_EQ(Directives[2].Kind, cxx_export_module_decl);
 }
 
 TEST(MinimizeSourceToDependencyDirectivesTest, ObjCMethodArgs) {

Bigcheese · 2025-07-14T17:44:21Z

clang/unittests/Lex/DependencyDirectivesScannerTest.cpp

@@ -1122,16 +1122,17 @@ ort \
    )";
  ASSERT_FALSE(
      minimizeSourceToDependencyDirectives(Source, Out, Tokens, Directives));
-  EXPECT_STREQ("#include \"textual-header.h\"\nexport module m;"
+  EXPECT_STREQ("module;#include \"textual-header.h\"\nexport module m;"


This needs a \n after the module; so that #include starts on a new line.

…ule directives Previously, the newline after a module directive was not properly captured (or printed). According to P1857R3, each directive must, after skipping horizontal whitespace, be at the start of a logical line. Because the newline after module directives was missing, this invalidated the following line. This also fixes or removes tests that were in violation of P1857R3. This extends to Objective-C directives, which should also follow P1857R3. This also ensures that the global module fragment `module;` is captured by the dependency directives scanner.

Bigcheese

Looks good to me, but I'd also like @akyrtzi to take a look if they have time.

akyrtzi · 2025-07-16T21:16:30Z

clang/unittests/Lex/DependencyDirectivesScannerTest.cpp

@@ -639,15 +639,12 @@ TEST(MinimizeSourceToDependencyDirectivesTest, AtImport) {
  ASSERT_FALSE(minimizeSourceToDependencyDirectives(" @ import  A;\n", Out));
  EXPECT_STREQ("@import A;\n", Out.data());

-  ASSERT_FALSE(minimizeSourceToDependencyDirectives("@import A\n;", Out));


Why is this check removed?

From P1857R3:

The entire import or module directive (including the closing ;) must be on a single logical line and for module must not come from an #include.

While this paper addresses C++, I've consulted with @Bigcheese on whether Objective-C modules should follow the same rules.
In the same spirit, a line-splice was added to this test:

llvm-project/clang/unittests/Lex/DependencyDirectivesScannerTest.cpp

Lines 645 to 647 in 586b438

ASSERT_FALSE(minimizeSourceToDependencyDirectives(

"@import /*x*/ A /*x*/ . /*x*/ B /*x*/ \\n /*x*/ ; /*x*/", Out));

EXPECT_STREQ("@import A.B\\n;\n", Out.data());

This patch currently fails CI due to some clang-scan-deps test failures caused by these changes. I'll fix those issues now. Apologies for missing this earlier.

Could you keep the check but update what the result of the scanner for this input should be?

Lines following the comment 'Note: Not required to pass CI' can be removed without the CI failing. The `TryConsumeToken(tok::eod)` is not required in those places, even though we know that a tok::eod follows. This is only for the patch review.

This commit removes the extra `TryConsumeToken(tok::eod)` which were added in e755d18 and which are not explicitly required for this fix or to pass the CI.

naveen-seth · 2025-07-18T02:15:25Z

clang/lib/Parse/Parser.cpp

+    // Note: Not required to pass CI
+    TryConsumeToken(tok::eod);


The patch passes the test-suite without the additional (commented) TryConsumeToken(tok::eod) calls added here, but since this patch adds a tok::eod at the end of every module directive, it also makes sense to consume it.

The next/newest commit (0bc7245) includes only the minimal needed changes and leaves out the unnecessary TryConsumeToken(tok::eod) calls.

I split this out into its own commit so I could get you feedback on this.

Bigcheese

This looks good to me, but I'd also like @yronglin to take a look since it interacts with their patch.

yronglin

LGTM! Thank you working on this.

naveen-seth · 2025-07-19T07:48:08Z

Thank you for reviewing!

llvm-ci · 2025-07-19T09:34:31Z

LLVM Buildbot has detected a new failure on builder lldb-aarch64-windows running on linaro-armv8-windows-msvc-05 while building clang at step 6 "test".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/141/builds/10268

Here is the relevant piece of the build log for the reference

Step 6 (test) failure: build (failure)
...
PASS: lldb-unit :: ValueObject/./LLDBValueObjectTests.exe/2/12 (2271 of 2280)
PASS: lldb-unit :: ValueObject/./LLDBValueObjectTests.exe/3/12 (2272 of 2280)
PASS: lldb-unit :: ValueObject/./LLDBValueObjectTests.exe/4/12 (2273 of 2280)
PASS: lldb-unit :: ValueObject/./LLDBValueObjectTests.exe/5/12 (2274 of 2280)
PASS: lldb-unit :: ValueObject/./LLDBValueObjectTests.exe/6/12 (2275 of 2280)
PASS: lldb-unit :: ValueObject/./LLDBValueObjectTests.exe/7/12 (2276 of 2280)
PASS: lldb-unit :: ValueObject/./LLDBValueObjectTests.exe/8/12 (2277 of 2280)
PASS: lldb-unit :: ValueObject/./LLDBValueObjectTests.exe/9/12 (2278 of 2280)
PASS: lldb-unit :: tools/lldb-server/tests/./LLDBServerTests.exe/0/1 (2279 of 2280)
TIMEOUT: lldb-unit :: Host/./HostTests.exe/6/12 (2280 of 2280)
******************** TEST 'lldb-unit :: Host/./HostTests.exe/6/12' FAILED ********************
Script(shard):
--
GTEST_OUTPUT=json:C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\build\tools\lldb\unittests\Host\.\HostTests.exe-lldb-unit-8128-6-12.json GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=12 GTEST_SHARD_INDEX=6 C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\build\tools\lldb\unittests\Host\.\HostTests.exe
--

Note: This is test shard 7 of 12.

[==========] Running 8 tests from 6 test suites.

[----------] Global test environment set-up.

[----------] 1 test from FileSystemTest

[ RUN      ] FileSystemTest.FileAndDirectoryComponents

[       OK ] FileSystemTest.FileAndDirectoryComponents (0 ms)

[----------] 1 test from FileSystemTest (0 ms total)



[----------] 1 test from HostInfoTest

[ RUN      ] HostInfoTest.GetAugmentedArchSpec

[       OK ] HostInfoTest.GetAugmentedArchSpec (0 ms)

[----------] 1 test from HostInfoTest (1 ms total)



[----------] 2 tests from MainLoopTest

[ RUN      ] MainLoopTest.ReadPipeObject


--
exit: 15

…dule directives (llvm#148685) Previously, the newline after a module directive was not properly captured and printed by `clang::printDependencyDirectivesAsSource`. According to P1857R3, each directive must, after skipping horizontal whitespace, appear at the start of a logical line. Because the newline after module directives was missing, this invalidated the following line. This fixes tests that were previously in violation of P1857R3, including for Objective-C directives, which should also comply with P1857R3. This also ensures that the global module fragment `module;` is captured by the dependency directives scanner.

[clang][deps] Recognize 'module;' in dependency directive scanner

7a75927

With this change, the dependency directive scanner now properly identifies "module;" as a directive, as per P1857R3. Previously, the global module fragment was not recognized by the scanner.

naveen-seth requested a review from Bigcheese July 14, 2025 17:37

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jul 14, 2025

Bigcheese reviewed Jul 14, 2025

View reviewed changes

naveen-seth added 2 commits July 14, 2025 19:53

Add newline after 'module;' to be valid C++

25553f7

naveen-seth changed the title ~~[clang][deps] Recognize 'module;' in dependency directive scanner~~ [clang][deps] Properly capture the global module and '\n' for all module directives Jul 16, 2025

Bigcheese requested a review from akyrtzi July 16, 2025 20:51

Bigcheese approved these changes Jul 16, 2025

View reviewed changes

akyrtzi reviewed Jul 16, 2025

View reviewed changes

naveen-seth added 3 commits July 18, 2025 03:50

Address akyrtzi's review feedback

e9a6b45

Update Parser and Preprocessor to handle the added tok::eod

0bc7245

This commit removes the extra `TryConsumeToken(tok::eod)` which were added in e755d18 and which are not explicitly required for this fix or to pass the CI.

naveen-seth requested review from Bigcheese and akyrtzi July 18, 2025 02:17

naveen-seth commented Jul 18, 2025

View reviewed changes

naveen-seth requested a review from yronglin July 18, 2025 18:00

Bigcheese approved these changes Jul 18, 2025

View reviewed changes

yronglin approved these changes Jul 19, 2025

View reviewed changes

naveen-seth merged commit 6855b9c into llvm:main Jul 19, 2025
9 checks passed

naveen-seth mentioned this pull request Jul 19, 2025

Reland [clang][modules-driver] Add scanner to detect C++20 module presence #147630

Merged

naveen-seth deleted the dep-directive-scanner-global-module branch July 20, 2025 14:45

This was referenced Jul 23, 2025

test abhinavgaba/llvm-project#2

Closed

Add dataFence plugin interface abhinavgaba/llvm-project#3

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clang][deps] Properly capture the global module and '\n' for all module directives #148685

[clang][deps] Properly capture the global module and '\n' for all module directives #148685

Uh oh!

naveen-seth commented Jul 14, 2025 •

edited

Loading

Uh oh!

llvmbot commented Jul 14, 2025

Uh oh!

Bigcheese Jul 14, 2025

Uh oh!

Bigcheese left a comment

Uh oh!

akyrtzi Jul 16, 2025

Uh oh!

naveen-seth Jul 16, 2025 •

edited

Loading

Uh oh!

akyrtzi Jul 16, 2025

Uh oh!

naveen-seth Jul 18, 2025 •

edited

Loading

Uh oh!

Bigcheese left a comment

Uh oh!

yronglin left a comment

Uh oh!

Uh oh!

naveen-seth commented Jul 19, 2025

Uh oh!

llvm-ci commented Jul 19, 2025

Uh oh!

Uh oh!

	ASSERT_FALSE(minimizeSourceToDependencyDirectives(
	"@import /x/ A /x/ . /x/ B /x/ \\n /x/ ; /x/", Out));
	EXPECT_STREQ("@import A.B\\n;\n", Out.data());

[clang][deps] Properly capture the global module and '\n' for all module directives #148685

[clang][deps] Properly capture the global module and '\n' for all module directives #148685

Uh oh!

Conversation

naveen-seth commented Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jul 14, 2025

Uh oh!

Bigcheese Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

Bigcheese left a comment

Choose a reason for hiding this comment

Uh oh!

akyrtzi Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

naveen-seth Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akyrtzi Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

naveen-seth Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bigcheese left a comment

Choose a reason for hiding this comment

Uh oh!

yronglin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

naveen-seth commented Jul 19, 2025

Uh oh!

llvm-ci commented Jul 19, 2025

Uh oh!

Uh oh!

naveen-seth commented Jul 14, 2025 •

edited

Loading

naveen-seth Jul 16, 2025 •

edited

Loading

naveen-seth Jul 18, 2025 •

edited

Loading